Skip to content

fix(node): prevent closeAndFlush from dropping in-flight events#1365

Merged
MichaelGHSeg merged 6 commits into
masterfrom
mgh/rate-limit-closeandflush-fix
Apr 27, 2026
Merged

fix(node): prevent closeAndFlush from dropping in-flight events#1365
MichaelGHSeg merged 6 commits into
masterfrom
mgh/rate-limit-closeandflush-fix

Conversation

@MichaelGHSeg

@MichaelGHSeg MichaelGHSeg commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • closeAndFlush data loss fix: The timeout handler silently swallowed errors (.catch(() => undefined)) and returned without cancelling in-flight retry loops. On process exit, pending retries were killed mid-flight. Now calls publisher.abort() via AbortController to cancel pending sleeps and mark affected batches as failed.
  • Default timeout floor: Raised from flushInterval * 1.25 (12.5s default) to Math.max(60s, flushInterval) * 1.25 (75s minimum), so the default timeout survives at least one Retry-After: 60 cycle.
  • http_response event: New emitter event for observing API response status codes and headers without monkey-patching fetch.
  • e2e-cli improvements: Node CLI now logs http_request/http_response. Browser CLI tracks Retry-After from 429s to avoid exiting while retries are pending.

Test plan

  • All 63 existing Node SDK tests pass (145 total, 1 pre-existing OAuth flake)
  • Updated default timeout test (75s) and added scaling test for large flushInterval
  • Verified 20/20 events delivered through staging rate limiting across Node, Browser, Swift, Kotlin, and React Native SDKs
  • Without fix: events silently dropped on closeAndFlush timeout (observed 13-18/20 delivery)
  • With fix: all events either delivered or explicitly marked as failed with error events

closeAndFlush had two issues that caused data loss under rate limiting:

1. The default timeout (flushInterval * 1.25 = 12.5s) was shorter than
   a single Retry-After: 60 cycle. Raised the floor to max(60s, flushInterval) * 1.25.

2. On timeout, .catch(() => undefined) silently swallowed the error and
   returned, but retry loops continued running. The process would then exit
   killing in-flight retries. Now calls publisher.abort() to cancel pending
   sleeps via AbortController, and marks affected batches as failed.
Adds an http_response event to the NodeEmitter contract and emits it
from the publisher after each API response. Enables consumers to
observe status codes, headers, and timing without monkey-patching fetch.
Node CLI: use http_request/http_response listeners for verbose logging.
Browser CLI: track Retry-After from 429 responses and use it as the
settle time, so the CLI doesn't exit while retries are still pending.
Both CLIs now respect config.timeout for their wait durations.
@changeset-bot

changeset-bot Bot commented Apr 24, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1e85dea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@segment/analytics-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov

codecov Bot commented Apr 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.51613% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.57%. Comparing base (0555f07) to head (1e85dea).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
packages/node/src/plugins/segmentio/publisher.ts 59.25% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1365      +/-   ##
==========================================
- Coverage   91.72%   91.57%   -0.15%     
==========================================
  Files         163      163              
  Lines        4674     4699      +25     
  Branches     1155     1157       +2     
==========================================
+ Hits         4287     4303      +16     
- Misses        387      396       +9     
Flag Coverage Δ
browser 92.51% <ø> (ø)
core 90.07% <ø> (ø)
node 89.43% <64.51%> (-0.93%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The previous logic forced a 60s minimum when Retry-After was 0 or
missing, causing e2e tests with short/zero Retry-After to time out.
Now uses the actual header value (default 10s when absent), so
Retry-After: 0 settles quickly while Retry-After: 60 waits properly.
With the new 75s closeAndFlush timeout floor, the "handles a bad key"
test would exceed the 30s jest timeout. Pass an explicit 20s timeout
and register resolveCtx before closeAndFlush so the error listener
catches the delivery failure event when abort fires.
@MichaelGHSeg
MichaelGHSeg merged commit 5eb12a1 into master Apr 27, 2026
16 checks passed
@MichaelGHSeg
MichaelGHSeg deleted the mgh/rate-limit-closeandflush-fix branch April 27, 2026 20:34
@github-actions github-actions Bot mentioned this pull request Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants